Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose way for request handlers to determine if the request came over HTTP or HTTPS. #420

Merged
merged 1 commit into from
Aug 12, 2022

Conversation

luqmana
Copy link
Contributor

@luqmana luqmana commented Aug 12, 2022

Added a new flag on DropshotState so that a request handler can determine what type of server it is currently running in. There's unfortunately no standard header or such in HTTP/1.1 (HTTP/2 does expose it via the :scheme pseudo-header).

Some motivation: In omicron, we currently stand up both an HTTP and HTTPS dropshot server with the same ApiEndpoints and thus handlers. For device auth, we need to construct absolute URIs for part of the response. We currently just have it hardcoded as http as we can't determine which to use. (*)

(*) Keeping both the http & https endpoints is temporary AFAIK so we could just change the hardcoded scheme later but the same premise still holds in general.

cc @plotnick

Copy link
Collaborator

@davepacheco davepacheco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks!

Thinking out loud: might it also make sense for us to provide an interface for getting the URL of the server at which we received the request (including http/https, host or IP address, and port)?

I wonder too if a consumer could achieve the same thing by adding a "tls" field in their server state object. i.e., maybe omicron could construct two slightly different server state objects, one with tls = true and one with tls = false.

None of this is a reason not to make this change. It seems useful either way.

Copy link
Contributor

@plotnick plotnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks!

@luqmana
Copy link
Contributor Author

luqmana commented Aug 12, 2022

The URL is already possible to grab off the Request object already available. Though, it also behaves differently depending on HTTP 1 v 2.

I wonder too if a consumer could achieve the same thing by adding a "tls" field in their server state object. i.e., maybe omicron could construct two slightly different server state objects, one with tls = true and one with tls = false.

Hehe, I originally was just gonna do that but in omicron they actually share the same server context via an Arc :P

@luqmana luqmana merged commit 8baf06e into main Aug 12, 2022
@luqmana luqmana deleted the luqmana/check-tls-in-handler branch August 12, 2022 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants